Skip to content

perf(physical-plan): fold PlanProperties fast-path into with_new_children_if_necessary (PR 1 of #22555)#23332

Merged
alamb merged 3 commits into
apache:mainfrom
zhuqi-lucas:qizhu/unify-with-new-children-if-necessary
Jul 10, 2026
Merged

perf(physical-plan): fold PlanProperties fast-path into with_new_children_if_necessary (PR 1 of #22555)#23332
alamb merged 3 commits into
apache:mainfrom
zhuqi-lucas:qizhu/unify-with-new-children-if-necessary

Conversation

@zhuqi-lucas

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part of #22555. This is PR 1 of 2 — see the issue body for the full plan. PR 2 will audit direct with_new_children callers and add a clippy lint.

Rationale for this change

Today the "skip work when children are unchanged" intent is split across two layers:

Having two independent layers means two places to maintain and two places for future changes to drift apart. This PR consolidates the fast-path into the single free-function helper so callers get both short-circuits uniformly.

What changes are included in this PR?

with_new_children_if_necessary now applies three layers, cheapest first:

  1. Same child pointers — every children[i] is Arc::ptr_eq to the corresponding existing child → return the original plan unchanged, no allocation.
  2. Same child properties — children's PlanProperties Arcs match → call the new ExecutionPlan::with_new_children_and_same_properties trait method to reuse the plan's PlanProperties cache without recomputing.
  3. Full recompute — otherwise, delegate to ExecutionPlan::with_new_children.

To make layer 2 dispatchable via &dyn ExecutionPlan, with_new_children_and_same_properties is promoted from an ad-hoc inherent method on each impl to a trait method with a safe default that falls back to with_new_children. All 22 existing impls migrate their inherent method to a trait override (mechanical change — signature &self → self: Arc<Self>, return Self → Result<Arc<dyn ExecutionPlan>>, body wrapped in Ok(Arc::new(...))).

The check_if_same_properties! macro and its call sites inside impls are kept, so direct callers of with_new_children (which PR 2 will audit + migrate) do not regress on this PR.

Are these changes tested?

Yes — added test_with_new_children_if_necessary_layers in execution_plan.rs that constructs test-local WithChildrenTestLeaf + WithChildrenTestParent plans (the parent tracks recompute vs fast-path calls via AtomicUsize) and asserts, for each of the three layers:

  • Layer 1: Arc::ptr_eq(result, parent) returns true, recompute_calls == 0, fast_path_calls == 0
  • Layer 2: Arc::ptr_eq(result.properties(), orig_props) returns true, recompute_calls == 0, fast_path_calls == 1
  • Layer 3: Arc::ptr_eq(result.properties(), orig_props) returns false, recompute_calls == 1, fast_path_calls unchanged

All 1523 datafusion-physical-plan unit tests pass. Full workspace cargo check + cargo clippy --all-targets --all-features -- -D warnings pass.

Are there any user-facing changes?

Yes — ExecutionPlan gains a new default-implemented trait method with_new_children_and_same_properties. Downstream impls that used to override the ad-hoc inherent method with the same name will need to re-implement as a trait override (mechanical signature change). Marking as api change.

Follow-up (PR 2, not in this PR)

  • Audit the ~47 remaining direct callers of plan.with_new_children(children) across the codebase and route them through with_new_children_if_necessary.
  • Add a disallowed_methods clippy lint (or custom lint) that forbids direct ExecutionPlan::with_new_children outside of a small allow-list.
  • Once all callers migrate, remove the check_if_same_properties! macro and its impl-side invocations, making the helper the single source of truth as described in the issue.

…dren_if_necessary

Route the "children are unchanged" short-circuit through a single helper
instead of two independent layers (caller-side Arc::ptr_eq check plus a
per-impl check_if_same_properties! fast-path from apache#19792).

with_new_children_if_necessary now applies three layers, cheapest first:
  1. same child pointers -> return the original plan unchanged
  2. same child PlanProperties Arcs -> reuse the plan's PlanProperties
     cache via with_new_children_and_same_properties
  3. otherwise -> full with_new_children recompute

To make layer 2 dispatchable through &dyn ExecutionPlan, promote
with_new_children_and_same_properties from an ad-hoc inherent method on
each impl to a trait method on ExecutionPlan with a safe default that
falls back to full recompute. All 22 existing impls migrate to overrides.

The check_if_same_properties! macro is kept for direct callers of
with_new_children until PR 2 audits them and adds a clippy lint (see
issue apache#22555 for the plan).

Part of apache#22555.
@github-actions github-actions Bot added the physical-plan Changes to the physical-plan crate label Jul 6, 2026
@zhuqi-lucas zhuqi-lucas marked this pull request as ready for review July 9, 2026 06:02
Copilot AI review requested due to automatic review settings July 9, 2026 06:03
@zhuqi-lucas zhuqi-lucas requested review from 2010YOUY01 and alamb July 9, 2026 06:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR consolidates the “skip work when rebuilding plans with unchanged children” logic into with_new_children_if_necessary by adding a new ExecutionPlan::with_new_children_and_same_properties trait method (defaulting to with_new_children) and migrating existing plan implementations to override it. It also updates the existing check_if_same_properties! macro to route through the new trait method and adds a unit test covering the intended 3-layer behavior.

Changes:

  • Add ExecutionPlan::with_new_children_and_same_properties as a trait method (default implementation falls back to with_new_children).
  • Update with_new_children_if_necessary to apply layered short-circuits (pointer-eq, properties-eq, then full recompute) and adjust has_same_children_properties to accept &dyn ExecutionPlan.
  • Mechanically migrate various ExecutionPlan impls to implement with_new_children_and_same_properties, plus add a new unit test covering the layered behavior.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
datafusion/physical-plan/src/execution_plan.rs Adds trait method + layered helper behavior + macro update + new unit test
datafusion/physical-plan/src/windows/window_agg_exec.rs Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/unnest.rs Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/union.rs Moves with_new_children_and_same_properties into ExecutionPlan trait impl (Union + Interleave)
datafusion/physical-plan/src/sorts/sort_preserving_merge.rs Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/sorts/partial_sort.rs Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/repartition/mod.rs Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/projection.rs Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/limit.rs Moves with_new_children_and_same_properties into ExecutionPlan trait impl (Global + Local)
datafusion/physical-plan/src/joins/symmetric_hash_join.rs Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/joins/sort_merge_join/exec.rs Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/joins/piecewise_merge_join/exec.rs Moves with_new_children_and_same_properties into ExecutionPlan trait impl and updates reset_state callsite
datafusion/physical-plan/src/joins/nested_loop_join.rs Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/joins/cross_join.rs Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/filter.rs Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/coop.rs Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/coalesce_partitions.rs Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/coalesce_batches.rs Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/buffer.rs Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/async_func.rs Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/aggregates/mod.rs Moves with_new_children_and_same_properties into ExecutionPlan trait impl

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread datafusion/physical-plan/src/execution_plan.rs
Comment thread datafusion/physical-plan/src/execution_plan.rs

@kosiew kosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhuqi-lucas

Looks good to me.
I just have one small, non-blocking suggestion that could help strengthen the test coverage.

///
/// Callers should route through [`with_new_children_if_necessary`] and
/// not invoke this method directly.
fn with_new_children_and_same_properties(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice change! One small suggestion: it might be worth adding a test for a plan that does not override with_new_children_and_same_properties, where the replacement child has the same PlanProperties Arc. That would help show that the default fallback still goes through with_new_children and keeps external or custom ExecutionPlan implementations on the semantics-preserving path. Totally non-blocking though. The existing override-path coverage and cargo check already look good.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion, thanks — done in e4328d9. Added WithChildrenTestParentDefault (a parent that intentionally does not override with_new_children_and_same_properties) plus test_with_new_children_if_necessary_default_fallback, which verifies that when the helper hits the same-properties branch on such a plan, the trait default forwards to with_new_children and the returned plan carries a freshly-recomputed PlanProperties Arc.

@zhuqi-lucas

Copy link
Copy Markdown
Contributor Author

@zhuqi-lucas

Looks good to me. I just have one small, non-blocking suggestion that could help strengthen the test coverage.

Thanks @kosiew for review!

Per @kosiew's review on apache#23332: cover the code path where a plan does
not override with_new_children_and_same_properties. The helper still
enters the 'same properties' branch, but the trait's default
implementation forwards to with_new_children, so downstream / external
ExecutionPlan implementations keep the semantics-preserving path even
without opting into the fast path.

@alamb alamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @zhuqi-lucas and @kosiew -- this does make sense to me though I have some thougths on the API design

children: Vec<Arc<dyn ExecutionPlan>>,
) -> Result<Arc<dyn ExecutionPlan>>;

/// Fast-path used by [`with_new_children_if_necessary`] when the new

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really have to add a new method to the trait? I am thinking of someone who goes to implement an ExecutionPlan and is bewildered by all the possible things that need to be implemented

For example, what if we deprecated with_new_children and renamed with_new_children_and_same_properties to soemthing like replace_children (e.g. that is clear it is different than with_new_children)

That way, in a few releases from now, ExecutionPlan can be trimmed down (people will have to implement replace_children and remove their implementation of with_new_children but at least ExecutionPlan is simpler in the long term

@zhuqi-lucas zhuqi-lucas Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alamb for review, good point — filed as follow-up in #23441 (canonical replace_children + deprecate both existing methods + migrate ~30 impls). Prefer to keep this PR focused on the semantic change so the API rename gets its own review thread.

@alamb alamb added this pull request to the merge queue Jul 10, 2026
Merged via the queue into apache:main with commit 24d8957 Jul 10, 2026
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants